ENH: Implementing Multivariate Rejection Sampling (MRS) in RocketPy #738
Conversation
Lucas-Prates
added
Enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #738 +/- ##
===========================================
- Coverage 76.42% 75.92% -0.50%
===========================================
Files 95 97 +2
Lines 11090 11198 +108
===========================================
+ Hits 8475 8502 +27
- Misses 2615 2696 +81 ☔ View full report in Codecov by Sentry. |
|
This PR is ready for a first "Design Review." I would like to get your opinion if this implementation provides what you think on how the user should use the MRS. Albeit a implementation as a function seems natural, I implemented as a class because:
It currently works as follows:
I provided a quick and dirty notebook, which will be removed, just to show how the class is being used at the moment. |
Lucas-Prates
requested review from
Gui-FernandesBR,
MateusStano and
phmbressan
Gui-FernandesBR
force-pushed
the
enh/MRS
branch
from
766bd58 to
c45fabf
Compare
There was a problem hiding this comment.
I don't have much time so I'll be short
- The class dies after you sample the data once, this makes it pointless to have a class
- Instead I'd remove the sample dictionary from arguments.
- First read the data during initialization. Then you use a function to set the variables you are going to allow be varied. This way u can already anticipate which variables may be varied.
- We want almost instant results for a MonteCarlo simulation after using MRS.
- Other thing is that the user must supply the original pdf. Could we possile estimate this from data? (imagine 70k)
- Finally, plotting is crucial for MRS, or even tables. We'd love to see more on that later.
Lucas-Prates
force-pushed
the
enh/MRS
branch
from
c45fabf to
d3ab5f2
Compare
| f"the monte carlo input file {input_filename}!" | ||
| ) from e | ||
|
|
||
| input_file.close() |
There was a problem hiding this comment.
This line should be inside a finally block, I believe. The same applies to the method load_output.
| """Loads input information from monte carlo in a SampleInformation | ||
| object | ||
| """ | ||
| input_filename = f"{self.monte_carlo_filepath}.inputs.txt" |
There was a problem hiding this comment.
If possible, it is better practice to handle paths with the pathlib standard library. Check out the parallel monte carlo PR as an example of handling the filename in the MonteCarlo class.
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas been updated (if relevant)New behavior
This PR implements the MRS requested in #162 and described in RocketPy paper.
Breaking change
Additional information
It is possibly a good idea to use setters when
distribution_dictis modified by the user. Otherwise, thesamplemethod can provide incorrect results due to attributes computed in the initialization.